-
Notifications
You must be signed in to change notification settings - Fork 471
Be more aware of the workspace during clean #7752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if something doesn't make any sense.
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
I think it might make sense to rethink the format command as well. Would it not be more consistent if we format either:
The |
Agreed, defaulting to all / keeping args similar to build/clean definitely makes sense! |
There still is more work to do here.
Running
I also think that if there is no link with package and repository rescript, For the time being if there is a workspace rescript.json, that "suffix" is primary. Post v12 we can explore the option to override suffix in child packages, but it is my understanding that it doesn't work that way today. I'm also in favour of renaming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the clean command to be workspace-aware by introducing a new ProjectContext
module that determines the project structure (single project, monorepo root, or monorepo package). The clean command now only removes files from the current project when run from within a monorepo package, rather than cleaning everything.
Key changes:
- Added
ProjectContext
enum to categorize project relationships and provide workspace-aware functionality - Modified clean command to only remove files from packages that are in scope based on the project context
- Updated format command to work with the new project context system and added dev dependency support
Reviewed Changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
rewatch/src/project_context.rs | New module defining ProjectContext enum and workspace relationship logic |
rewatch/src/build/clean.rs | Updated clean function to use ProjectContext for scoped cleaning |
rewatch/src/format.rs | Modified to use ProjectContext and support dev dependency formatting |
rewatch/src/cli.rs | Updated format command CLI to remove --all flag and add dev flag |
rewatch/src/main.rs | Updated format command invocation |
rewatch/src/helpers.rs | Updated helper functions to work with ProjectContext |
rewatch/src/build/packages.rs | Refactored package reading to work with ProjectContext |
rewatch/tests/clean.sh | New test file for clean command functionality |
rewatch/tests/format.sh | Updated format tests to work with new CLI |
rewatch/testrepo/* | Test repository updates to support workspace testing |
@@ -1,4 +1,4 @@ | |||
{ | |||
"name": "rescript", | |||
"dependencies": ["@tests/gentype-react-example"] | |||
"dependencies": ["@tests/gentype-react-example", "playground"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"playground"
is part of the workspace, so I need to be listed here for rewatch to pick up the correct node_modules
folder.
The package.json
has rescript clean
in the commands.
This will now accurately only clean the playground package.
But it needs that link to the parent rescript json.
bold "Rescript version" | ||
(cd ../testrepo && ./node_modules/.bin/rescript -v) | ||
(cd ../testrepo && ./node_modules/.bin/rescript --version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-v
is verbosity and this was trying to compile the testrepo with an older installed version of rescript.
I'm not sure how this wasn't leading to bigger problems.
// I don't think this linting rule matters very much as we only create a single instance of this enum. | ||
// See https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant | ||
#[allow(clippy::large_enum_variant)] | ||
pub enum ProjectContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This streamlines the many workspace: Option<string>
checks that were being done.
It improves the readability and debugability of rewatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe do:
pub enum MonoRepoContext {
Root {local_dependencies, local_dev_dependencies}
Package {parent_config: Config, parent_path: RescriptJsonPath}
}
pub struct ProjectContext {
config: Config,
path: RescriptJsonPath,
mono_repo_context: Option<MonoRepoContext>
}
I would like to be mindful about memory consumption - even if minor - you never know how much these will grow in the future - and I think we should try and keep a small footprint (smaller than we do now). I think the enum
will otherwise always take the biggest size when allocating, even in smaller contexts.
For those overwhelmed by these changes: in short, this resolves the expectations described in #7707 comment. This PR makes Rewatch more aware of its context:
In the last case, Rewatch will run I removed the I will add a changelog entry if there is consensus to move forward. Please ask any questions you may have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work! Left some comments on the Rust bits and pieces 👌 / some optimisations.
@@ -709,7 +699,7 @@ fn compile_file( | |||
} | |||
|
|||
// copy js file | |||
root_package.config.get_package_specs().iter().for_each(|spec| { | |||
root_config.get_package_specs().iter().for_each(|spec| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of a note to myself for some future refactor - I think we can do better with memory management - the package specs / root_config and those things are essentially constant after reading. We should be able - for the most part - to just always return references rather than cloning - I think we can further smallen the footprint / make things quite a bit faster that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function also get's called a lot. I think we might benefit from some more abstraction, having an internal representation for a package / packagespec, rather than the parseTree we need for Serde
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 usages indeed, worth revisiting. Will consider this outside the scope of this PR, as it is already so large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we pass references right? Where are we doing cloning?
// I don't think this linting rule matters very much as we only create a single instance of this enum. | ||
// See https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant | ||
#[allow(clippy::large_enum_variant)] | ||
pub enum ProjectContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe do:
pub enum MonoRepoContext {
Root {local_dependencies, local_dev_dependencies}
Package {parent_config: Config, parent_path: RescriptJsonPath}
}
pub struct ProjectContext {
config: Config,
path: RescriptJsonPath,
mono_repo_context: Option<MonoRepoContext>
}
I would like to be mindful about memory consumption - even if minor - you never know how much these will grow in the future - and I think we should try and keep a small footprint (smaller than we do now). I think the enum
will otherwise always take the biggest size when allocating, even in smaller contexts.
rewatch/src/project_context.rs
Outdated
dependencies | ||
.iter() | ||
.map(|dep| format!(" \"{dep}\"")) | ||
.collect::<Vec<String>>() | ||
.join(",\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format
+ iter
+ join
can be done more efficiently with write!
without any additional intermediate overhead;
let mut output = String::from("[\n{");
for dep of dependencies {
fmt::write(&mut output, format!(" \"{dep}\,\n"")).expect("Error");
}
fmt::write(&mut output, "}\n]").expect("Error");
output
If you really want to fly - I think multiple write
operations in the loop would be even faster than serializing the dep
with formatter, as it's already a string - but I'm not quite sure how much faster it would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - I see it's used within fmt
only - perhaps push it into that context, then we'll know it's not used for anything outside of formatting / performance matters less
rewatch/src/project_context.rs
Outdated
let context = match nearest_parent_config_path { | ||
None => monorepo_or_single_project(&path, current_rescript_json, current_config), | ||
Some(parent_config_path) => { | ||
match packages::read_config(parent_config_path.as_path()) { | ||
Err(e) => Err(anyhow!( | ||
"Could not read the parent config at {}: {}", | ||
parent_config_path.to_string_lossy(), | ||
e | ||
)), | ||
Ok((workspace_config, workspace_rescript_json)) => { | ||
let is_current_config_listed_in_workspace = workspace_config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try to maybe remove the nesting ever so slightly here - nesting multiple matches. Perhaps break out the config reading to another function as well?
context | ||
} | ||
|
||
pub fn get_root_config(&self) -> &Config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking up the enum
into a struct
for the shared fields would remove the need for these too, which is nice imo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not exactly the same, because we want to get different things (in a mono repo package, get the root config, otherwise get the config of the current package). So I think it's pretty nice like this - and with an implementation, it's easy to get the configuration.
rewatch/src/project_context.rs
Outdated
for dep in local_dependencies { | ||
local_packages.insert(dep.clone()); | ||
} | ||
if include_dev_deps { | ||
for dep in local_dev_dependencies { | ||
local_packages.insert(dep.clone()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe there is an extend
function or something like that.
I'm not sure what the underlying representation of the hashset is, but a bulk operation will be more optimized. So probably something like:
let deps = local_dev_dependencies.into_iter().collect::AHashSet<String>();
local_packages.extend(deps)
Is faster because the CPU can plan ahead better.
// See https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant | ||
#[allow(clippy::large_enum_variant)] | ||
pub enum ProjectContext { | ||
/// Single project - no workspace relationship |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nice, I like this a lot!
This is an attempt to drive the conversation at #7707
In case of clean, we already do an upward traversal to find a root rescript json.
I'm adding some logic to check if the parent rescript json contains the current rescript json in "dependencies" (or "dev-dependencies").
If we did find a link, we would only remove the files from the current project and not everything.
Consider https://github.com/nojaf/rescript-kaplay/blob/main/packages/rescript-kaplay/rescript.json
We already find https://github.com/nojaf/rescript-kaplay/blob/d38649fcd409c174c39b31488f790e036f3b0a20/rescript.json#L8
If that name matches
root_package
(in the code, a bit confusing name) is"@nojaf/rescript-kaplay"
, so we only need to remove those files.Files from https://github.com/nojaf/rescript-kaplay/blob/main/packages/samples/rescript.json are unaffected.
If we were to clean from the repo root,
root_config
would be the well the root package and everything gets cleaned.